feat: Adding Expiration Column to API Tokens#2421
feat: Adding Expiration Column to API Tokens#2421RaymondLaubert wants to merge 2 commits intomainfrom
Conversation
…dated the AuthToken struct to include Expiration. BED-7449
RaymondLaubert
added
enhancement
|
Howdy! Thank you for opening this pull request 🙇 Your title is formatted correctly but we did not find a matching issue reference. Details: |
📝 WalkthroughWalkthroughIntroduces token expiration support by adding an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.7.0.sql (1)
150-152: Consider indexingauth_tokens.expires_atfor the newly added query path.Line 152 adds
expires_at, and token sorting/filtering now includes"expires_at"incmd/api/src/model/auth.go(Line 191, Line 209). On larger datasets, this can degrade to full scans/sorts without an index.💡 Suggested migration addition
ALTER TABLE auth_tokens ADD COLUMN IF NOT EXISTS expires_at timestamp with time zone; + +CREATE INDEX IF NOT EXISTS idx_auth_tokens_expires_at + ON auth_tokens (expires_at);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/migration/migrations/v8.7.0.sql` around lines 150 - 152, Add an index on the new auth_tokens.expires_at column to avoid full-table scans during sorting/filtering; update the v8.7.0 migration to CREATE INDEX CONCURRENTLY IF NOT EXISTS on auth_tokens(expires_at) (or a partial index if you only query non-null/unrevoked tokens) so code paths that reference "expires_at" in the auth model use the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/model/auth.go`:
- Line 154: The Expiration field in the auth model is missing an explicit GORM
column mapping which causes GORM to target "expiration" instead of the DB column
"expires_at"; update the struct field Expiration (in cmd/api/src/model/auth.go)
to include a gorm tag mapping to column:expires_at (e.g., add
gorm:"column:expires_at" alongside the existing json tag) so reads/writes use
the correct database column.
---
Nitpick comments:
In `@cmd/api/src/database/migration/migrations/v8.7.0.sql`:
- Around line 150-152: Add an index on the new auth_tokens.expires_at column to
avoid full-table scans during sorting/filtering; update the v8.7.0 migration to
CREATE INDEX CONCURRENTLY IF NOT EXISTS on auth_tokens(expires_at) (or a partial
index if you only query non-null/unrevoked tokens) so code paths that reference
"expires_at" in the auth model use the index.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/database/migration/migrations/v8.7.0.sqlcmd/api/src/model/auth.go
| Key string `json:"key,omitempty"` | ||
| HmacMethod string `json:"hmac_method"` | ||
| LastAccess time.Time `json:"last_access"` | ||
| Expiration null.Time `json:"expires_at"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AuthToken field/tag check =="
rg -n -C2 'type AuthToken struct|Expiration|null.Time|expires_at|gorm:"column:expires_at"' cmd/api/src/model/auth.go
echo
echo "== GORM naming strategy check (repo-wide) =="
rg -n -C3 'gorm\.Config|NamingStrategy|schema\.NamingStrategy|NameReplacer|ColumnName' --type=go cmd/api/src
echo
echo "== Column name references for auth_tokens =="
rg -n -C2 '\bauth_tokens\b|\bexpires_at\b|\bexpiration\b' cmd/api/srcRepository: SpecterOps/BloodHound
Length of output: 14522
Add explicit GORM column mapping to prevent mismatch with database schema.
At line 154, the Expiration field lacks a gorm:"column:expires_at" tag. GORM's default naming convention converts Expiration to expiration, but the migration creates the column as expires_at. This mismatch will cause reads and writes on this field to target the wrong column.
Fix
- Expiration null.Time `json:"expires_at"`
+ Expiration null.Time `json:"expires_at" gorm:"column:expires_at"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Expiration null.Time `json:"expires_at"` | |
| Expiration null.Time `json:"expires_at" gorm:"column:expires_at"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/model/auth.go` at line 154, The Expiration field in the auth
model is missing an explicit GORM column mapping which causes GORM to target
"expiration" instead of the DB column "expires_at"; update the struct field
Expiration (in cmd/api/src/model/auth.go) to include a gorm tag mapping to
column:expires_at (e.g., add gorm:"column:expires_at" alongside the existing
json tag) so reads/writes use the correct database column.
There was a problem hiding this comment.
The other way to fix this is to use the same name: ExpiresAt instead of Expiration
There was a problem hiding this comment.
@superlinkx You're absolutely right! Renaming the field to ExpiresAt is a better solution. It follows the existing convention in the codebase (e.g., AuthSecret.ExpiresAt) and is more idiomatic. GORM will automatically convert ExpiresAt to expires_at via snake_case, matching the database column.
This approach is cleaner and more consistent with the rest of the codebase. The StripKey() method would also need updating to use ExpiresAt instead of Expiration:
func (s AuthToken) StripKey() AuthToken {
return AuthToken{
UserID: s.UserID,
ClientID: s.ClientID,
Key: "",
HmacMethod: s.HmacMethod,
LastAccess: s.LastAccess,
Unique: s.Unique,
Name: s.Name,
ExpiresAt: s.ExpiresAt,
}
}✏️ Learnings added
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 2421
File: cmd/api/src/model/auth.go:154-154
Timestamp: 2026-02-25T23:53:13.603Z
Learning: In the BloodHound codebase (SpecterOps/BloodHound), time-based expiration fields should be named `ExpiresAt` (not `Expiration`) to follow existing conventions (e.g., `AuthSecret.ExpiresAt`) and to allow GORM's automatic snake_case conversion to correctly map to `expires_at` database columns without requiring explicit `gorm:"column:expires_at"` tags.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: brandonshearin
Repo: SpecterOps/BloodHound PR: 2383
File: cmd/api/src/database/kind.go:61-69
Timestamp: 2026-02-25T20:04:47.906Z
Learning: In the BloodHound codebase (SpecterOps/BloodHound), GORM v1.25.x is used. For Raw queries with IN clauses that take slices, the correct syntax is `IN (?)` with parentheses (e.g., `WHERE id IN (?)`), not `IN ?`. This pattern is used consistently across the codebase in files like cmd/api/src/database/kind.go and cmd/api/src/database/sourcekinds.go.
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: packages/go/stbernard/command/audit/audit.go:133-136
Timestamp: 2025-08-26T21:25:33.480Z
Learning: superlinkx prefers to receive feedback and identify issues as early as possible in the review process rather than later.
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 2382
File: cmd/api/src/api/v2/ad_related_entity.go:44-57
Timestamp: 2026-02-23T17:06:23.991Z
Learning: In BloodHound, AD nodes are represented by a base kind (ad.Entity) plus specific kinds (ad.User, ad.Computer, ad.Group, etc.), and Azure nodes by az.Entity. The query.KindIn function matches if a node has any of the specified kinds, so using the base kind (ad.Entity) will correctly include all AD nodes regardless of their specific subtype. During reviews, ensure code that filters or queries graph nodes via KindIn uses the base kind to capture all inherited subkinds for AD nodes, and similarly recognizes base kinds for Azure nodes. This design pattern should be consistently respected across files that model or query graph schema kinds.
superlinkx
left a comment
superlinkx
left a comment
There was a problem hiding this comment.
Just a quick sanity check plus CodeRabbit's issue, otherwise this looks correct for the ticket
|
|
||
| -- Update the 'auth_tokens' Table adding Expiration Column | ||
| ALTER TABLE auth_tokens | ||
| ADD COLUMN IF NOT EXISTS expires_at timestamp with time zone; No newline at end of file |
There was a problem hiding this comment.
We want to treat this as a nullable field, but this is not creating it as nullable
2 participants
Description
Updated the auth_token table to include a column for the expiration date of the token.
Motivation and Context
Resolves BED-7449
This change adds the expiration column to the auth_tokens table allowing an expiration date to be set for an API token. This further expands on allowing API tokens to be set to expire; which clients have requested.
How Has This Been Tested?
Must be tested further. Placeholder for now.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit